Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NOJIRA fix body consumption in http4s #160

Merged
merged 3 commits into from
Jul 9, 2024

Conversation

fserra-mdsol
Copy link
Contributor

As reported on this http4s issue, in some conditions, if the request's EntityBody is consumed more than once by the caller, a BodyAlreadyConsumedError is raised. In our MAuthMiddleware, this can happen when performing the mAuth authentication on large or slow requests, whereby the request's Stream might be chunked, requiring multiple consumptions and causing intermittent failures.

This PR aims at solving this bug

@fserra-mdsol fserra-mdsol requested a review from amilne-mdsol July 8, 2024 17:41
* request body is now consumed in one go and stored in memory

* prevent occurrences of BodyAlreadyConsumedError
@fserra-mdsol fserra-mdsol force-pushed the NOJIRA-fix_body_consumption_in_http4s branch from e8c2b22 to bf42749 Compare July 8, 2024 17:42
tmilner
tmilner previously approved these changes Jul 9, 2024

authenticator.authenticate(req)(requestValidationTimeout).map(res => (res, authCtx))
fk(for {
strictBody <- request.toStrict(none)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, toStrict enforces a request size limit doesn't it? Could pass that in as an option (potentially with a default to keep it backwards compatible?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, it doesn't enforce any limit - it would be recommended to enforce one, because toStrict only consumes the whole EntityBody and replaces any multipart header with a content-length (as a testament to its function), but there is no fixed size limit. In practice, this means that your application might blow up if the request is too large to be held in memory while it gets deserialised. I decided to not put any 'feature flag' on this, because I gauged that for our use cases there is no such risk - but if you know that it's not the case otherwise, I can add it.

@tmilner tmilner dismissed their stale review July 9, 2024 08:24

Question added

@tmilner tmilner merged commit 62a8187 into master Jul 9, 2024
2 checks passed
@fserra-mdsol fserra-mdsol deleted the NOJIRA-fix_body_consumption_in_http4s branch July 9, 2024 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants